feat: add parallel chunk processing for large documents in transformations - #529

2 issues found across 6 files

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="open_notebook/utils/token_utils.py">

<violation number="1" location="open_notebook/utils/token_utils.py:153">
P2: `is_context_limit_error` matches generic substrings like "limit" and "exceeded", so common rate-limit errors (e.g., "Rate limit exceeded") will be treated as context-length errors. In transformation, that triggers parallel chunk retries, likely worsening rate limiting and causing retry spikes.</violation>

<violation number="2" location="open_notebook/utils/token_utils.py:268">
P2: Sentence-level splitting can still create chunks that exceed max_tokens when a single sentence is longer than the limit, violating the function’s contract and potentially re-triggering context-limit errors.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

1 issue found across 2 files (changes from recent commits).

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="open_notebook/utils/token_utils.py">

<violation number="1" location="open_notebook/utils/token_utils.py:310">
P2: `current_chunk` is set to a list of words, but remaining chunks are joined with `"\n\n"`, so an oversized sentence fragment at the end will be emitted with double newlines between every word instead of spaces.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

Thanks @kevincolten — this is a genuinely nice piece of work: the optimistic-full → chunk-on-context-error → parallel Send → hierarchical synthesis flow is well structured, it keeps Prompter intact, doesn't disturb the token_count fix, and it's well tested (the 145K-token real run is a great touch). Two blocking items before it can merge, plus a few smaller notes.

Blocking

1. Output cap silently drops from 8192 → 4096 on the normal (non-chunked) path.
try_full_content sets output_buffer = DEFAULT_OUTPUT_TOKENS (4096) and calls provision_langchain_model(..., max_tokens=output_buffer). main currently uses max_tokens=8192. Since try_full runs for every transformation (not just large docs), any transformation whose output exceeds 4096 tokens would now be truncated — a silent regression on the common path. Please preserve the current 8192 default for the full-content attempt (or make the output budget configurable and default it to 8192).

2. Module-level asyncio.Semaphore bound at import.

_chunk_semaphore = asyncio.Semaphore(_CHUNK_CONCURRENCY_LIMIT)  # module scope

There are no other module-level asyncio primitives in the codebase, and this one is a footgun: an asyncio.Semaphore binds to the first event loop it's awaited from. This graph is imported by the worker (run_transformation_command) and can also be exercised from the API's loop; awaiting the same module-level semaphore from two loops raises RuntimeError: bound to a different event loop. Please create it inside the function (or lazily per-invocation) rather than at import.

Non-blocking (worth addressing)

3. Context-limit detection parses provider error strings (is_context_limit_error / get_context_limit_from_error). That's inherently provider-format-dependent and will silently fall back to DEFAULT_CONTEXT_LIMIT (8192) when a wording doesn't match, which can mis-size chunks. A short comment on the supported formats + how the fallback behaves would help future maintenance.

4. Chunk meta-prefix leaks into content. Each chunk is sent as "[Processing section X of N from a larger document]\n\n{chunk}". For summarization that's fine, but for extraction-style transformations that instruction text can bleed into or skew the output. Consider putting the "section X of N" hint in the system prompt instead of the user content.

5. _CHUNK_CONCURRENCY_LIMIT = 3 is a second concurrency layer on top of the worker's own limit (OPEN_NOTEBOOK_WORKER_MAX_TASKS, see #893). Worth a comment noting the interaction, or deriving it from the same config.

Process note

There's no linked issue — features go through an approved issue first (CONTRIBUTING). The feature itself is well-aligned, so this is easy to formalize; I'll get an issue opened to track it. Once (1) and (2) are addressed I'm happy to re-review. (Heads up: maintainerCanModify is off on this PR, so these need to come from your side.)

All reported issues were addressed across 3 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

All reported issues were addressed across 1 file (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

Thanks for keeping this alive and for addressing both blockers from the last round (8192 default restored, per-event-loop semaphore). Issue #990 is ready and this is the implementation, so let's get it over the line. Two things remain, both small, and one is newly important:

1. Wrap the chunk-path LLM calls in classify_error(). process_chunk and _synthesize_once let raw provider exceptions escape. #1275 (merged this week) added ContextLengthExceededError to the retry blocklists in commands/source_commands.py, so an unclassified exception from a chunk node will now be retried by surreal-commands with the full exponential budget, which is exactly the behavior #1275 removed. Route them through the same classify_error → raise error_class(...) from e pattern the single-shot path uses in open_notebook/graphs/transformation.py.
2. Reuse open_notebook/utils/error_classifier.py instead of a second keyword list. token_utils.is_context_limit_error duplicates the context-limit detection main already owns; the two will drift. Build one on the other.

Two non-blocking notes for your judgment: calculate_output_buffer(8192) yields ~819 output tokens per chunk when the error wording doesn't parse, which seems likely to truncate chunk results; and the synthesis prompt ("merge, remove redundancy") is right for summaries but lossy for extraction-style transformations, silently. A sentence in the docs about that trade-off would be enough.

Please rebase on main and add a CHANGELOG line under Unreleased → Added.

1 issue found across 9 files (changes from recent commits).

Confidence score: 4/5

• open_notebook/utils/token_utils.py may stop recognizing some provider context-limit errors when the message says only “exceeds the maximum,” causing the expected context-length handling or fallback to be skipped—restore the previously supported wording in is_context_limit_error().

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="open_notebook/utils/token_utils.py">

<violation number="1" location="open_notebook/utils/token_utils.py:186">
P2: When a provider reports a context rejection as `exceeds the maximum` without `context` or `input token count`, `is_context_limit_error()` now returns false. Add the previously supported context wording to the shared classifier rule so these errors still trigger chunking.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

1 issue found across 2 files (changes from recent commits).

Confidence score: 5/5

• In tests/test_context_length_no_retry.py, test_status_codes_still_match_as_standalone_numbers now also contains token-rate throttle cases, which mixes unrelated concerns and could make failures harder to diagnose; move those cases into a dedicated test.

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/test_context_length_no_retry.py">

<violation number="1" location="tests/test_context_length_no_retry.py:113">
P3: The two new token-rate throttle cases were appended to `test_status_codes_still_match_as_standalone_numbers`, a test whose stated concern (and name) is that status codes like 429 match only as standalone numbers. These cases contain no status code; they pin token-throttle wording to RateLimitError instead. They test a different concern and read better as their own parametrized test (or a rename), so a classification regression in either path isn't attributed to the right guard.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>